feat: extension point for myst options #115

tavin · 2023-03-20T19:55:15Z

No description provided.

github-actions · 2023-03-20T19:55:28Z

👈 Launch a Binder on branch tavin/jupyterlab-myst/feat/myst-options

rowanc1

Took a look through the PR, haven't tested it out locally yet though!

@agoose77 I would be curious on your thoughts on the extension mechanism, I am not that familiar in that area at all!

rowanc1 · 2023-03-21T17:36:10Z

src/MySTMarkdownCell.tsx

+    if (this._doRendering) this.renderInput(null as unknown as Widget);
+    this._doRendering = true;


Is this because downstream you are calling this constructor?

Do we need this render in here at all? Or can we remove it + the option and explicitly call renderInput when we need to?

It's all just a horrible hack because of a bug in the 3.5.x parent class. A cursory look tells me it's fixed in the 3.6.x series. I don't know when it's okay for jupyterlab-myst to start following v3.6. But once the bug is gone, this code can be gone.

rowanc1 · 2023-03-21T17:37:56Z

src/myst.ts

+export interface MySTOptions {
+
+  parserOptions: Partial<AllOptions>;
+}


@fwkoch is there any thinking we need to on this before we start encouraging using these more widely?!

They look pretty solid I think.

rowanc1 · 2023-03-21T17:38:42Z

src/myst.ts

+export class MySTNotebookDefaults implements MySTOptionsProvider<StaticNotebook> {
+
+  get(notebook: StaticNotebook): MySTOptions {
+    return {
+      parserOptions: {
+        directives: [cardDirective, gridDirective, ...tabDirectives],
+        roles: [evalRole],
+      },
+    };
+  }
+}


I would love to see you are overriding things with other extensions @tavin!

Here: https://github.com/tavin/myst-options-poc/tree/main/src

rowanc1 · 2023-03-21T17:39:46Z

src/myst.ts

-    directives: [cardDirective, gridDirective, ...tabDirectives],
-    roles: [evalRole]
-  });
+export function markdownParse(text: string, options: Partial<AllOptions>): Root {


Should options be potentially undefined here (and other places)?

Suggested change

export function markdownParse(text: string, options: Partial<AllOptions>): Root {

export function markdownParse(text: string, options?: Partial<AllOptions>): Root {

I am newish to typescript, actually I didn't know that was available syntax.... but anyway, I don't know? Why?

tavin · 2023-03-26T17:17:22Z

@rowanc1 I got all the checks passing except "PR label" which it seems I'm not allowed to edit.

tavin · 2023-04-19T07:39:28Z

@rowanc1 Does this need a deeper discussion with stakeholders to move it along? I am not sure where or with whom to pursue that... but I think I can explain why these changes make sense :)

agoose77 · 2023-04-26T18:45:35Z

At a high level, this looks reasonable, thanks @tavin! Some points I'm not clear on:

why does the interface accept a notebook widget? Is that a use case you already have a need for?
@rowanc1 what's the benefit from defining directives without defining the renderer? Is it just to map new directives onto existing mdast so that they can be rendered?

tavin · 2023-04-27T21:17:05Z

@agoose77

why does the interface accept a notebook widget? Is that a use case you already have a need for?

I can imagine several ways of choosing myst options:

through a jupyterlab settings page, handled by a standard labextension (maybe part of jupyterlab-myst, or in any case maintained by executablebooks?) ... this being the use case that doesn't need the notebook reference
notebook metadata (why not, if someone wants to write a labextension for it?)
reading a _config.yml file reachable from the notebook file (another labextension possibility which I might attempt)

When the markdown preview integration is ready I am thinking my approach would be duplicated with some kind of document object in place of the notebook object. Then for me the 3rd bullet point would be great for working with jupyter books "live" i.e. without constantly rebuilding and reloading the output.

To be fair I'm not 100% sure how to use the notebook (document) object for these purposes but I read over some code and API docs and it seemed to me that it should be possible.

A simpler argument is that it's probably wrong to assume that globally fixed myst options are correct for every project/file a user edits. The notebook (document) object is the obvious thing to switch on.

agoose77 · 2023-05-12T11:08:52Z

OK, I circled back around to look at this!

I'm a bit hesitant in merging this because I have not yet thought about what the extension picture should look like. In part, that's because the question of extensibility goes beyond just making jupyterlab-myst work, and into defining these extensions alongside the document.

Furthermore, there are questions as to whether extensions should be per-document, or per-environment (I think the former).

Maybe we can start by having discussions with @stevejpurves @rowanc1 et al. to figure out how we want to capture this information.

I can see a fairly straightforward solution, which involves defining a frontmatter entry that contains a table of reserved extension names. Then, the frontend can be in charge of ensuring that those extensions are loaded (i.e. some kind of plugin system). There's also a question of versioning, but this extends beyond plugins to the MyST distribution itself.

What happens for third-party extensions is less clear; I am worried about the fragmentation that comes from this, but it's also important to help people use MyST as they need it.

tavin · 2023-05-12T11:47:26Z

I think this particular PR should be viewed as merely adding a technical path forward so that someone could build a jupyterlab settings page where people can opt into colon fences or whatever.

For me and @meldefon well we just wanna be able to edit documents with sphinx proof directives and see something that looks like an admonition in the markdown preview. If this PR is merged I can very easily hack that into my local environment with a labextension -- that I can share -- while we wait for sphinx proof to land in mystjs.

Also I've hinted at wanting to be able to detect what myst options are set in my jupyter book build and mimic those. Why not?

I am not trying to create a myst-markdown extension/plugin architecture right now :)

agoose77 · 2023-05-12T12:25:24Z

For me and @meldefon well we just wanna be able to edit documents with sphinx proof directives and see something that looks like an admonition in the markdown preview. If this PR is merged I can very easily hack that into my local environment with a labextension -- that I can share -- while we wait for sphinx proof to land in mystjs.

OK, this is a good argument. Right now, it's really helpful that you're testing these things out. Long term, I don't think this PR is the right way forward, but I don't want to hold things back in the near term. So, I propose that we merge it as an experimental interface that may be removed in future. Does this work for you? You can always pin to the version of jupyterlab-myst that contains this code to ensure it always works for you.

tavin · 2023-05-12T13:17:12Z

I'd be happy with an experimental interface and indeed I think it should be clearly documented as experimental.

In the future I'll be delighted if it's replaced with something better. What matters is the philosophy:

that labextension authors have some way to tune myst options
that it's possible to tune myst options based on the file or project (so not only from frontmatter -- e.g. we don't want to repeat the same _config.yml boilerplate in every markdown file in a project)

Thank you!

agoose77 · 2023-05-15T11:43:49Z

@tavin I'll get to this early this week :)

extension point for myst options

280c8a1

tavin mentioned this pull request Mar 20, 2023

Syntax extensions should be configurable #25

Open

Merge branch 'main' into feat/myst-options

a305824

rowanc1 reviewed Mar 21, 2023

View reviewed changes

tavin added 2 commits March 24, 2023 01:41

Merge branch 'executablebooks:main' into feat/myst-options

38270d4

compliance with "prettier"

c53884a

tavin force-pushed the feat/myst-options branch from 9fa002f to c53884a Compare March 26, 2023 16:11

compliance with eslint

4bea826

tavin marked this pull request as ready for review March 26, 2023 17:21

rowanc1 added the enhancement New feature or request label Mar 27, 2023

tavin mentioned this pull request Apr 26, 2023

Does this work? tavin/myst-options-poc#1

Open

agoose77 changed the title ~~extension point for myst options~~ feat: extension point for myst options Apr 26, 2023

This was referenced May 3, 2023

fix styles and scrolling + pin some dependencies agoose77/jupyterlab-myst#2

Closed

trim deps and upgrade to @jupyterlab 3.6 #137

Closed

Merge branch 'executablebooks:main' into feat/myst-options

f9f9ed6

tavin added 3 commits May 16, 2023 11:15

a bit more documentation

e560686

Merge branch 'main' into feat/myst-options

d163036

wrong again

e7705fe

rowanc1 mentioned this pull request Jun 1, 2023

Consider how users can extend roles/directives #45

Open

rowanc1 force-pushed the main branch from be183a5 to 29de3e7 Compare June 11, 2023 21:09

agoose77 force-pushed the main branch from e73c81a to 25ddbcd Compare February 6, 2024 14:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: extension point for myst options #115

feat: extension point for myst options #115

tavin commented Mar 20, 2023

github-actions bot commented Mar 20, 2023

rowanc1 left a comment

rowanc1 Mar 21, 2023

tavin Mar 21, 2023 •

edited

Loading

rowanc1 Mar 21, 2023

rowanc1 Mar 21, 2023

tavin Mar 26, 2023

rowanc1 Mar 21, 2023

tavin Mar 21, 2023

tavin commented Mar 26, 2023

tavin commented Apr 19, 2023

agoose77 commented Apr 26, 2023

tavin commented Apr 27, 2023

agoose77 commented May 12, 2023 •

edited

Loading

tavin commented May 12, 2023

agoose77 commented May 12, 2023

tavin commented May 12, 2023

agoose77 commented May 15, 2023

		if (this._doRendering) this.renderInput(null as unknown as Widget);
		this._doRendering = true;

	export function markdownParse(text: string, options: Partial<AllOptions>): Root {
	export function markdownParse(text: string, options?: Partial<AllOptions>): Root {

feat: extension point for myst options #115

Are you sure you want to change the base?

feat: extension point for myst options #115

Conversation

tavin commented Mar 20, 2023

github-actions bot commented Mar 20, 2023

rowanc1 left a comment

Choose a reason for hiding this comment

rowanc1 Mar 21, 2023

Choose a reason for hiding this comment

tavin Mar 21, 2023 • edited Loading

Choose a reason for hiding this comment

rowanc1 Mar 21, 2023

Choose a reason for hiding this comment

rowanc1 Mar 21, 2023

Choose a reason for hiding this comment

tavin Mar 26, 2023

Choose a reason for hiding this comment

rowanc1 Mar 21, 2023

Choose a reason for hiding this comment

tavin Mar 21, 2023

Choose a reason for hiding this comment

tavin commented Mar 26, 2023

tavin commented Apr 19, 2023

agoose77 commented Apr 26, 2023

tavin commented Apr 27, 2023

agoose77 commented May 12, 2023 • edited Loading

tavin commented May 12, 2023

agoose77 commented May 12, 2023

tavin commented May 12, 2023

agoose77 commented May 15, 2023

tavin Mar 21, 2023 •

edited

Loading

agoose77 commented May 12, 2023 •

edited

Loading